Wizard/Packages: Sort packages by the newest added (HMS-10767) - #4719
Wizard/Packages: Sort packages by the newest added (HMS-10767)#4719regexowl wants to merge 1 commit into
Conversation
Codecov Report✅ All modified and coverable lines are covered by tests. @@ Coverage Diff @@
## main #4719 +/- ##
==========================================
+ Coverage 78.00% 78.11% +0.11%
==========================================
Files 264 264
Lines 7069 7033 -36
Branches 2597 2576 -21
==========================================
- Hits 5514 5494 -20
+ Misses 1458 1442 -16
Partials 97 97
Flags with carried forward coverage won't be shown. Click here to find out more.
... and 2 files with indirect coverage changes Continue to review full report in Codecov by Harness.
🚀 New features to boost your workflow:
|
There was a problem hiding this comment.
Hey - I've left some high level feedback:
- The new
return [...packages].reverse();andreturn [...groups].reverse();logic assumes the incoming arrays are always ordered oldest → newest; if that’s a requirement, consider making it explicit (e.g., sorting by a timestamp field) or documenting it so future changes to the source ordering don’t silently break the “newest first” behavior. - Previously groups were sorted alphabetically by name, but now they are simply reversed; if the UX doesn’t explicitly call for recency-based ordering of groups, it may be worth double‑checking that reversing is intentional and not an accidental regression from the prior deterministic ordering.
Prompt for AI Agents
Please address the comments from this code review:
## Overall Comments
- The new `return [...packages].reverse();` and `return [...groups].reverse();` logic assumes the incoming arrays are always ordered oldest → newest; if that’s a requirement, consider making it explicit (e.g., sorting by a timestamp field) or documenting it so future changes to the source ordering don’t silently break the “newest first” behavior.
- Previously groups were sorted alphabetically by name, but now they are simply reversed; if the UX doesn’t explicitly call for recency-based ordering of groups, it may be worth double‑checking that reversing is intentional and not an accidental regression from the prior deterministic ordering.Help me be more useful! Please click 👍 or 👎 on each comment and I'll use the feedback to improve your reviews.
a7d46d2 to
716aa8b
Compare
| } | ||
| }; | ||
|
|
||
| const sortedPackages = useMemo(() => { |
There was a problem hiding this comment.
The same sorting logic remains in PackageSearch where it still makes sense. In the case of packages table, there is no reason to sort the rows by stream, end date, etc. and thus adding the packages to the selected packages table in seemingly random places.
716aa8b to
b22288f
Compare
There was a problem hiding this comment.
Hey - I've found 1 issue, and left some high level feedback:
- The removal of
sortedGroupsmeans package groups are now rendered in their insertion order instead of alphabetically; if group ordering is still expected to be deterministic for users, consider preserving a simple name-based sort or explicitly documenting the new behavior. - The reducer now relies on
unshiftto represent recency, but the UI doesn’t explicitly sort by a timestamp or other field; if future changes alter insertion patterns, it may be safer to encode recency in the data model rather than relying solely on array position.
Prompt for AI Agents
Please address the comments from this code review:
## Overall Comments
- The removal of `sortedGroups` means package groups are now rendered in their insertion order instead of alphabetically; if group ordering is still expected to be deterministic for users, consider preserving a simple name-based sort or explicitly documenting the new behavior.
- The reducer now relies on `unshift` to represent recency, but the UI doesn’t explicitly sort by a timestamp or other field; if future changes alter insertion patterns, it may be safer to encode recency in the data model rather than relying solely on array position.
## Individual Comments
### Comment 1
<location path="src/store/slices/wizard/content/tests/content.test.ts" line_range="66" />
<code_context>
expect(state.content.packages.map((p) => p.name)).toEqual([
- 'vim',
- 'git',
'curl',
+ 'git',
+ 'vim',
]);
});
</code_context>
<issue_to_address>
**issue (testing):** Add corresponding tests for `groups` reducer to cover newest-first behaviour
The expectations for `packages` now correctly reflect the newest-first (`unshift`) behaviour. The same ordering change was applied to `groups`, but there’s no test asserting this.
Please add reducer tests for `addPackageGroup` that start from a non-empty `groups` array, add one or more groups, and verify that new groups are inserted at the beginning. This will keep coverage aligned with the reducer behaviour and guard against regressions in `groups` ordering.
</issue_to_address>Help me be more useful! Please click 👍 or 👎 on each comment and I'll use the feedback to improve your reviews.
| expect(state.content.packages.map((p) => p.name)).toEqual([ | ||
| 'vim', | ||
| 'git', | ||
| 'curl', |
There was a problem hiding this comment.
issue (testing): Add corresponding tests for groups reducer to cover newest-first behaviour
The expectations for packages now correctly reflect the newest-first (unshift) behaviour. The same ordering change was applied to groups, but there’s no test asserting this.
Please add reducer tests for addPackageGroup that start from a non-empty groups array, add one or more groups, and verify that new groups are inserted at the beginning. This will keep coverage aligned with the reducer behaviour and guard against regressions in groups ordering.
b22288f to
f9a0fe5
Compare
Sorting by recency is intentional.
Information about packages / pkg groups is stored in a flat list of names, so we need to work with that. |
bc1e9d2 to
2d8be1d
Compare
This replaces original sorting logic, that was mostly leftover from the pre-revamp table focused workflow, with a simple sort by recency. This will not touch sorting within the search, so all functionality as sorting active app stream to the top within the list of available options is still in place.
2d8be1d to
38aa41a
Compare
This replaces original sorting logic, that was mostly leftover from the pre-revamp table focused workflow, with a simple sort by recency.
This will not touch sorting within the search, so all functionality as sorting active app stream to the top within the list of available options is still in place.